Cache computation of JSON nodes to improve performance #995
Conversation
…terparts; cache all low level funcs
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files@@ Coverage Diff @@
## main #995 +/- ##
==========================================
- Coverage 70.25% 61.56% -8.70%
==========================================
Files 62 62
Lines 4472 4483 +11
==========================================
- Hits 3142 2760 -382
- Misses 1330 1723 +393 ☔ View full report in Codecov by Sentry. |
| @@ -92,12 +92,12 @@ def validate_json_node_keys(node: Mapping[str, Any]): | |||
| ) | |||
|
|
|||
|
|
|||
| @guidance(stateless=True) | |||
| @guidance(stateless=True, cache=True) | |||
There was a problem hiding this comment.
If adding cache here gives a speed up, will this result in every generated integer being the same?
There was a problem hiding this comment.
I'll certainly double check before marking this as non-WIP, but I believe it's just caching the return value of this function, which is a GrammarFunction, not the actual string value. In other words, I believe the answer to your question is no
There was a problem hiding this comment.
Related question, assuming that the return value is OK: does this get cross-referenced properly when the grammar is generated? Looking at some of the serialisation code, I expect that it does, but would be good to make sure that it doesn't get expanded inline multiple times.
There was a problem hiding this comment.
cache=True will ensure that subsequent calls with the same arguments will always return the exact same object (i.e. id(o1) == id(o2)).
The serialization code maintains its own cache for handling "pointers" (just integer indices) to objects in the serialized grammar, as you can see here. GrammarFunctions are hashable and just hash to the id of the object, so this code should correctly respect the object caching we do in the decorator.
|
@riedgar-ms |
…ike container object (#1007) Essentially reopening #638 with some major simplifications allowed by the rust re-write (the serialized grammar now directly supports "references"). Reopening because: #995 still takes far too long to process the large JSON schema. Line-profiling revealed that ~80% of the time spent constructing the `GrammarFunction` is due to `replace_grammar_node`. In particular, this function has to traverse the entire grammar tree, and we potentially have to call it many times if there is a lot of mutual recursion. Simply adding a node type that acts as a container (which we can fill after we decide what its contents should be) side-steps this problem entirely.
hudson-ai
changed the title
|
An open question: is top/module-level caching a good idea? I could add a layer of encapsulation such that all of the underscore prefixed functions only maintain a cache within a single call to Also, feeling pretty okay about the |
You might want to cross check with Paul, since he's been doing the async/multithreaded stuff. |
|
@paulbkoch would love your input on the caching business here :) |
|
Currently, the performance gain on actually constructing the grammar doesn't seem super affected by this PR (I think thanks to #1007, which provided a significant boost on its own). However, caching is still needed -- otherwise we hit a limit on the number of symbols that |
The one scenario I could think of where per-json call caching might be superior is in the case of a long running workload with constantly changing JSON schemas that are unique per call. In that case module level caching will keep consuming more and more memory. Given the JSON schemas and grammars are pretty compact, it would probably take a long time to exhaust memory though, so it's probably not the most important thing to solve. In terms of async, I can't think of an issue this caching would create, at least not after your other PR on fixing threaded grammars @hudson-ai. The rest of the PR looked fine to me. I can't say that I understood all the JSON schema nuances here, but the application of caching to the existing code looked good to me. |
Agreed. If I can get my other PR on method decoration together, it would make it really easy to cache on a per-schema basis (one schema = one instance of a GenJSON class, methods are cached at the instance level...). Just want to reiterate that this introduces a new dependency in |
WIP. Seeing 4x speedup on large nested schema after adding caching, but we can probably squeeze more performance out of this. Not sure if the
frozendictdep is strictly necessary; just trying out a quick idea.